feat: implement timeout handling for Redis key retrieval #1472
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements a timeout mechanism for Redis key retrieval operations to prevent blocking on large datasets. The implementation adds two new private methods: getAllKeysWithTimeout wraps the Redis keys() command with a timeout, and getAllKeysWithScan provides a fallback using the non-blocking SCAN command when timeouts occur.
Key Changes:
- Replaced direct
keys('*')call ingetTablesFromDBwith timeout-wrapped version - Added timeout handling with configurable 5-second default
- Implemented SCAN-based fallback for timeout scenarios
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is trailing whitespace at the end of this line.
| } | |
| } |
| private async getAllKeysWithScan(redisClient: RedisClientType, pattern: string = '*'): Promise<string[]> { | ||
| const allKeys: string[] = []; | ||
| const scanOptions = { MATCH: pattern, COUNT: 1000 }; | ||
| let cursor = '0'; | ||
|
|
||
| do { | ||
| const result = await redisClient.scan(cursor, scanOptions); | ||
| cursor = result.cursor.toString(); | ||
| allKeys.push(...result.keys); | ||
| } while (cursor !== '0'); | ||
|
|
||
| return allKeys; | ||
| } |
There was a problem hiding this comment.
The new getAllKeysWithScan method lacks test coverage. Given that this is a critical fallback mechanism for timeout scenarios, it should have tests covering: normal iteration with multiple SCAN calls, handling of empty results, and proper cursor management.
|
|
||
| do { | ||
| const result = await redisClient.scan(cursor, scanOptions); | ||
| cursor = result.cursor.toString(); |
There was a problem hiding this comment.
The explicit conversion result.cursor.toString() may be unnecessary depending on the Redis client library's return type. The cursor is typically already a string from the SCAN command. Consider verifying the type returned by the Redis client and removing the conversion if redundant.
| cursor = result.cursor.toString(); | |
| cursor = result.cursor; |
| const keysPromise = redisClient.keys('*'); | ||
|
|
||
| const timeoutPromise = new Promise<null>((resolve) => { | ||
| setTimeout(() => resolve(null), timeoutMs); | ||
| }); | ||
|
|
||
| const result = await Promise.race([keysPromise, timeoutPromise]); | ||
|
|
||
| if (result === null) { | ||
| return this.getAllKeysWithScan(redisClient); | ||
| } | ||
|
|
||
| return result; |
There was a problem hiding this comment.
The timeout mechanism has a critical flaw: when the timeout occurs, the original keys() operation continues running in the background without cancellation. This can lead to resource leaks and unnecessary load on the Redis server. Additionally, if the keys() call fails with an error, it will reject the Promise.race and bypass the timeout fallback to SCAN. Consider wrapping the keysPromise in a try-catch or implementing proper cancellation/cleanup of the timed-out operation.
| const keysPromise = redisClient.keys('*'); | |
| const timeoutPromise = new Promise<null>((resolve) => { | |
| setTimeout(() => resolve(null), timeoutMs); | |
| }); | |
| const result = await Promise.race([keysPromise, timeoutPromise]); | |
| if (result === null) { | |
| return this.getAllKeysWithScan(redisClient); | |
| } | |
| return result; | |
| let timeoutHit = false; | |
| let timeoutHandle: NodeJS.Timeout; | |
| const timeoutPromise = new Promise<null>((resolve) => { | |
| timeoutHandle = setTimeout(() => { | |
| timeoutHit = true; | |
| resolve(null); | |
| }, timeoutMs); | |
| }); | |
| try { | |
| const keysPromise = redisClient.keys('*'); | |
| const result = await Promise.race([keysPromise, timeoutPromise]); | |
| clearTimeout(timeoutHandle); | |
| if (result === null) { | |
| // Timeout hit, fallback to SCAN | |
| return this.getAllKeysWithScan(redisClient); | |
| } | |
| return result; | |
| } catch (err) { | |
| // If keysPromise rejects, fallback to SCAN | |
| if (timeoutHandle) { | |
| clearTimeout(timeoutHandle); | |
| } | |
| return this.getAllKeysWithScan(redisClient); | |
| } |
|
|
||
| private async getAllKeysWithTimeout( | ||
| redisClient: RedisClientType, | ||
| timeoutMs: number = 5000, |
There was a problem hiding this comment.
The magic number 5000 (5 seconds) for the default timeout is not explained. Consider extracting this to a named constant with documentation explaining why this specific timeout value was chosen, or making it configurable based on environment or connection parameters.
| private async getAllKeysWithTimeout( | ||
| redisClient: RedisClientType, | ||
| timeoutMs: number = 5000, | ||
| ): Promise<string[]> { | ||
| const keysPromise = redisClient.keys('*'); | ||
|
|
||
| const timeoutPromise = new Promise<null>((resolve) => { | ||
| setTimeout(() => resolve(null), timeoutMs); | ||
| }); | ||
|
|
||
| const result = await Promise.race([keysPromise, timeoutPromise]); | ||
|
|
||
| if (result === null) { | ||
| return this.getAllKeysWithScan(redisClient); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The getAllKeysWithTimeout method lacks documentation. It should have a JSDoc comment explaining the timeout mechanism, the fallback behavior to SCAN when timeout occurs, and the rationale for the default timeout value.
| private async getAllKeysWithTimeout( | ||
| redisClient: RedisClientType, | ||
| timeoutMs: number = 5000, | ||
| ): Promise<string[]> { | ||
| const keysPromise = redisClient.keys('*'); | ||
|
|
||
| const timeoutPromise = new Promise<null>((resolve) => { | ||
| setTimeout(() => resolve(null), timeoutMs); | ||
| }); | ||
|
|
||
| const result = await Promise.race([keysPromise, timeoutPromise]); | ||
|
|
||
| if (result === null) { | ||
| return this.getAllKeysWithScan(redisClient); | ||
| } | ||
|
|
||
| return result; | ||
| } |
There was a problem hiding this comment.
The new getAllKeysWithTimeout method lacks test coverage. Given the complexity of the timeout and race condition handling, it should have tests covering: successful completion before timeout, timeout triggering fallback to SCAN, and error handling when keys() fails.
| redisClient: RedisClientType, | ||
| timeoutMs: number = 5000, | ||
| ): Promise<string[]> { | ||
| const keysPromise = redisClient.keys('*'); |
There was a problem hiding this comment.
The getAllKeysWithTimeout method uses a hardcoded '' pattern in redisClient.keys('*'), while the fallback method getAllKeysWithScan accepts a pattern parameter. This inconsistency means if getAllKeysWithScan is enhanced in the future to support different patterns, the timeout path would still use ''. Consider adding a pattern parameter to getAllKeysWithTimeout and passing it to both the keys() call and the getAllKeysWithScan fallback.
| const keysPromise = redisClient.keys('*'); | ||
|
|
||
| const timeoutPromise = new Promise<null>((resolve) => { | ||
| setTimeout(() => resolve(null), timeoutMs); | ||
| }); | ||
|
|
||
| const result = await Promise.race([keysPromise, timeoutPromise]); | ||
|
|
||
| if (result === null) { |
There was a problem hiding this comment.
There's a race condition in the timeout handling. If the keys() operation completes just after the timeout fires but before Promise.race resolves, both operations could complete nearly simultaneously, potentially causing undefined behavior. The implementation should ensure proper handling of this edge case.
| const keysPromise = redisClient.keys('*'); | |
| const timeoutPromise = new Promise<null>((resolve) => { | |
| setTimeout(() => resolve(null), timeoutMs); | |
| }); | |
| const result = await Promise.race([keysPromise, timeoutPromise]); | |
| if (result === null) { | |
| let timeoutFired = false; | |
| const keysPromise = redisClient.keys('*'); | |
| const timeoutPromise = new Promise<null>((resolve) => { | |
| setTimeout(() => { | |
| timeoutFired = true; | |
| resolve(null); | |
| }, timeoutMs); | |
| }); | |
| const result = await Promise.race([keysPromise, timeoutPromise]); | |
| if (result === null) { | |
| // Ensure any late rejection of keysPromise is handled | |
| keysPromise.catch(() => {}); |
|
|
||
| do { | ||
| const result = await redisClient.scan(cursor, scanOptions); | ||
| cursor = result.cursor.toString(); | ||
| allKeys.push(...result.keys); | ||
| } while (cursor !== '0'); | ||
|
|
There was a problem hiding this comment.
The SCAN operation lacks error handling. If any SCAN call fails during iteration, the entire operation will throw an unhandled error. This should be wrapped in try-catch to ensure graceful error handling, especially since this is used as a fallback for when the regular keys() operation times out.
| do { | |
| const result = await redisClient.scan(cursor, scanOptions); | |
| cursor = result.cursor.toString(); | |
| allKeys.push(...result.keys); | |
| } while (cursor !== '0'); | |
| try { | |
| do { | |
| const result = await redisClient.scan(cursor, scanOptions); | |
| cursor = result.cursor.toString(); | |
| allKeys.push(...result.keys); | |
| } while (cursor !== '0'); | |
| } catch (error) { | |
| // Log the error and return an empty array or the keys collected so far | |
| console.error('Error during Redis SCAN operation:', error); | |
| return []; | |
| } |
No description provided.